Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rpc: make inactive_stake consistent in getStakeActivation #35116

Closed

Conversation

CriesofCarrots
Copy link
Contributor

Problem

The RPC endpoint getStakeActivation only looks at the total stake balance for the deactivating and inactive cases. This means that if an active stake account receives a transfer, the amount is not reflected in the "inactive" stake, which is confusing.

Summary of Changes

Evaluate inactive_stake to the total lamports balance minus effective stake and rent-exempt reserve for all activation cases.

Fixes #35111

}
};
let inactive_stake = stake_account
.lamports()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm... won't this be misleading in a situation where lamports are deposited while a delegation is incurring a multi-epoch warmup/cooldown? since those lamports are never going to be active without first deactivating the delegation.

i think considering lamports() for anything purporting to be "stake" is probably wrong


it seems like what we really want to communicate here is something like withrawable_balance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talk about this a little on the issue that was created: #35111 (comment)
In one sense, it is misleading, but I actually don't think it's incorrect to say that the additional lamports are inactive even though they will not ever be activated (without first deactivating).

The stake: deactivating case already reports extra lamports as inactive, so there is an inconsistency here that ought to be fixed, regardless. I suppose another way to do that would be to update the deactivating case to calculate the inactive from the Delegation amount instead of the account balance. But that seems less helpful to me.

All that said, since we no longer support past epochs for this method (and since we don't return the actual activating/deactivating amounts), users can figure out everything this method returns by inspecting the stake account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talk about this a little on the issue that was created: #35111 (comment)

eh... my gut says "reporter is holding it wrong". i'm not a huge fan of reinforcing more wrong conceptual interpretations. we have too many already. we screwed ourselves by synonymizing stake and delegation.

In one sense, it is misleading, but I actually don't think it's incorrect to say that the additional lamports are inactive even though they will not ever be activated (without first deactivating).

i don't disagree that excess lamports in a stake account are inactive. i just don't agree that they are "stake" in the first place.

The stake: deactivating case already reports extra lamports as inactive, so there is an inconsistency here that ought to be fixed, regardless. I suppose another way to do that would be to update the deactivating case to calculate the inactive from the Delegation amount instead of the account balance. But that seems less helpful to me.

yeah this is kinda what i was getting at here:

i think considering lamports() for anything purporting to be "stake" is probably wrong

removing lamports() from any of these calculations seems like it would only improve the accuracy of the response. i'd rather answer "why don't my excess lamports show up in the getStakeActivation?" and correct the wrong expectation than further muddy it

All that said, since we no longer support past epochs for this method (and since we don't return the actual activating/deactivating amounts), users can figure out everything this method returns by inspecting the stake account.

so deprecate this method and call it a day? :trollface:

Copy link

@xdzurman xdzurman Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't disagree that excess lamports in a stake account are inactive. i just don't agree that they are "stake" in the first place.

  1. Why would they not be stake? They are funds on a stake account.
  2. Why would you consider it stake, if the funds were sent to an inactive/deactivating stake account (they would be shown in inactive_stake) but not if the funds were sent to an active/activating stake account (they're hidden)?

yeah this is kinda what i was getting at here:

As carrot guy said, that would be far less helpful and confusing to handle in my opinion too

it seems like what we really want to communicate here is something like withrawable_balance?

I guess that would kinda fix the issue, but it would create even more confusion as to what is considered stake, what is not, what is withdrawable, whether withdrawable === inactive in some cases and not in others, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so deprecate this method and call it a day? :trollface:

Yes, I'm actually fine with that, but I would like to fix the inconsistency as well. My preferred fix is the one in this PR, so I will add a deprecation commit (although deprecation communication mostly happens on the docs side).

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (7a95e4f) to head (51039cb).
Report is 94 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #35116       +/-   ##
===========================================
+ Coverage        0    81.6%    +81.6%     
===========================================
  Files           0      831      +831     
  Lines           0   224830   +224830     
===========================================
+ Hits            0   183551   +183551     
- Misses          0    41279    +41279     

@CriesofCarrots
Copy link
Contributor Author

CriesofCarrots commented Feb 7, 2024

CI failure unrelated to this patch... er, I mean I needed to rebase. First commit is unchanged.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 23, 2024
@CriesofCarrots CriesofCarrots removed the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 23, 2024
@willhickey
Copy link
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

Copy link
Contributor

mergify bot commented Mar 4, 2024

⚠️ The sha of the head commit of this PR conflicts with #35419. Mergify cannot evaluate rules on this PR. ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getStakeActivation rpc method incorrect inactive balance
4 participants